Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add create/load/refereshRepository() methods to IRepositoryManager #311

Merged
merged 4 commits into from
Sep 21, 2023

Conversation

HannesWell
Copy link
Member

Add the following methods to the super interface IRepositoryManager, which both existing sub-interfaces IArtifactRepositoryManager and IMetadataRepositoryManager covariantly override:

  • createRepository()
  • loadRepository()
  • refreshRepository()

This allows more general usage of the IRepositoryManager interface, for example in Tycho.

Furthermore (in the first of the two commits), apply a few minor doc unifications and clean-ups in all RepositoryManager interfaces.

@github-actions
Copy link

github-actions bot commented Sep 1, 2023

Test Results

       9 files  ±0         9 suites  ±0   38m 18s ⏱️ + 2m 45s
2 175 tests ±0  2 171 ✔️ ±0    4 💤 ±0  0 ±0 
6 615 runs  ±0  6 604 ✔️ ±0  11 💤 ±0  0 ±0 

Results for commit e3bce54. ± Comparison against base commit 344ac78.

♻️ This comment has been updated with latest results.

Copy link
Member

@laeubi laeubi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only think I can think of is that a repository manager not necessarily must support creation/loading/ of repositories and these must not necessarily even be URI based...

Also I wonder if this can be used in P2 anywhere directly?

Copy link
Contributor

@merks merks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine.

@HannesWell
Copy link
Member Author

The only think I can think of is that a repository manager not necessarily must support creation/loading/ of repositories and these must not necessarily even be URI based...

At least at the moment there are only two sub-interfaces, which both do support it and override these methods.
I cannot assess if it is realist to assume that in the future there might be another sub-type that does not support that?
But in case we now say no falsely, such implementation can still throw an UnsupportedOperationException like it is for example done in the Collections framework.

Also I wonder if this can be used in P2 anywhere directly?

I checked it and indeed there are many places and I added another commit to use the new methods.

It also unifies the behavior if a repository manager of specific type cannot be found in ProvisioningHelper to throw an exception in all cases. But I assume those services should actually never be absent.

The little difficulty is that the generic IRepositoryManager can only return an IRepository<IInstallableUnit> instead of IMetadataRepository and IRepository<IArtifactKey> instead of IArtifactRepository but often the specializations are assumed which requires casting. In order to cover that with generics, the list of parameters of IRepositoryManager<T> would have to be extended to IRepositoryManager<T, R extends IRepository<T>>.
But this would at least be a source-incompatible change (which is annoying but solvable, because if one compiles code one can usually adapt it).

A review of the changes in the third commit would be more than welcome.

@HannesWell HannesWell force-pushed the repoManagers branch 2 times, most recently from 64e67a0 to 2ed4e1d Compare September 2, 2023 21:01
// remove the repo first.
mgr.removeRepository(toInit.getRepoLocation());

// first try and load to see if one already exists at that location.
try {
IMetadataRepository repository = mgr.loadRepository(toInit.getRepoLocation(), null);
if (!validRepositoryLocation(repository) && initDestinationRepository(repository, toInit))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validRepositoryLocation() either returned true or threw an exception.
Therefore initDestinationRepository() was never called for a metadata repository.

@laeubi
Copy link
Member

laeubi commented Sep 19, 2023

@HannesWell can we merge this one?

Add the following methods to the super interface IRepositoryManager,
which both existing sub-interfaces IArtifactRepositoryManager and
IMetadataRepositoryManager covariantly override:
- createRepository()
- loadRepository()
- refreshRepository()

This allows more general usage of the IRepositoryManager interface
This also unifies the behavior if a repository manager of specific type
cannot be found in ProvisioningHelper.
@HannesWell HannesWell merged commit 5e1da67 into eclipse-equinox:master Sep 21, 2023
9 checks passed
@HannesWell HannesWell deleted the repoManagers branch September 21, 2023 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants